Skip to content

fix(cli): prevent root rejection when using canCallTool with bypassPermissions#180

Open
cruzanstx wants to merge 1 commit intotiann:mainfrom
cruzanstx:fix/root-permission-mode-bypass
Open

fix(cli): prevent root rejection when using canCallTool with bypassPermissions#180
cruzanstx wants to merge 1 commit intotiann:mainfrom
cruzanstx:fix/root-permission-mode-bypass

Conversation

@cruzanstx
Copy link
Copy Markdown

Summary

  • Root cause fix: When running as root, Claude Code rejects --dangerously-skip-permissions (mapped from --permission-mode bypassPermissions). Since canCallTool already handles permission auto-approval via --permission-prompt-tool stdio, skip passing the redundant --permission-mode flag in that case.
  • Abort error fix: Change if/if to if/else if in close handler to prevent exit code error from overwriting AbortError
  • Stderr capture: Always log stderr via logDebug() instead of only when DEBUG env is set — critical for diagnosing silent Claude process exits
  • Remote launcher logging: Log actual error name/message/stack instead of empty {} (Error objects don't JSON.stringify)
  • Sync engine fallback: Fall back to first online machine instead of returning "No machine online" error when no host-specific match is found

Context

When hapi runs as root (common in containerized/server deployments), the --yolo / bypassPermissions mode causes Claude Code to immediately exit with code 1 and the message --dangerously-skip-permissions cannot be used with root/sudo privileges. The remote launcher catches this as an empty error {} and shows "Process exited unexpectedly" to the user.

The fix recognizes that canCallTool + --permission-prompt-tool stdio already achieves the same effect as bypassPermissions — permissions are auto-approved app-side — so the redundant CLI flag can be safely omitted.

Test plan

  • Run hapi as root with --yolo mode and verify Claude sessions start successfully
  • Verify non-root users are unaffected
  • Verify abort (Ctrl-C) still works correctly and doesn't show spurious exit code errors
  • Verify stderr from Claude process appears in hapi debug logs

🤖 Generated with Claude Code

if (hostMatch) return hostMatch
}
return null
return onlineMachines[0]
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MAJOR] [Correctness] Resume can target wrong machine

Why this is a problem: When metadata.machineId or metadata.host is set but no match is online, this now falls back to the first online machine. Resume then uses metadata.path on a different machine, risking wrong workspace or unintended session creation.

Suggested fix:

const targetMachine = (() => {
    if (metadata.machineId) {
        const exact = onlineMachines.find((machine) => machine.id === metadata.machineId)
        if (exact) return exact
        return null
    }
    if (metadata.host) {
        const hostMatch = onlineMachines.find((machine) => machine.metadata?.host === metadata.host)
        if (hostMatch) return hostMatch
        return null
    }
    return onlineMachines[0]
})()

if (!targetMachine) {
    return { type: 'error', message: 'No matching machine online', code: 'no_machine_online' }
}

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Resume can target wrong machine when metadata.machineId or metadata.host mismatches; fallback to first online uses metadata.path on another machine. Evidence hub/src/sync/syncEngine.ts:353.
    Suggested fix:
    const targetMachine = (() => {
        if (metadata.machineId) {
            const exact = onlineMachines.find((machine) => machine.id === metadata.machineId)
            if (exact) return exact
            return null
        }
        if (metadata.host) {
            const hostMatch = onlineMachines.find((machine) => machine.metadata?.host === metadata.host)
            if (hostMatch) return hostMatch
            return null
        }
        return onlineMachines[0]
    })()
    
    if (!targetMachine) {
        return { type: 'error', message: 'No matching machine online', code: 'no_machine_online' }
    }

Summary

  • 1 issue. Resume safety risk on multi-machine namespaces.

Testing

  • Not run (automation)

…rmissions

When running as root, Claude Code rejects --dangerously-skip-permissions
(mapped from --permission-mode bypassPermissions). Since canCallTool already
handles permission auto-approval via --permission-prompt-tool stdio, skip
passing the redundant --permission-mode flag in that case.

Also fixes abort error being overwritten by exit code error (else-if),
captures stderr via logDebug for diagnostics, improves error logging in
remote launcher, and falls back to first online machine in sync engine.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@tiann tiann force-pushed the fix/root-permission-mode-bypass branch from dc2f094 to 5dfea4e Compare March 3, 2026 02:19
Copy link
Copy Markdown
Collaborator

@hqhq1025 hqhq1025 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix — the root + bypassPermissions rejection is a real issue. I ran into a related problem in #406 (now merged) where exit_plan_mode handling depends on canCallTool being the permission mechanism, so your Change 1 logic is architecturally correct.

However, this PR bundles three unrelated changes and has gone stale. Here's my review per change:


Change 1: query.ts — skip --permission-mode when canCallTool + bypassPermissions

Logic is correct — when canCallTool is provided, HAPI handles permissions via the stdio protocol. Passing --dangerously-skip-permissions is redundant and crashes under root.

But the diff is stale. PR #343 (merged 2026-03-23) rewrote large parts of `query.ts` — the stderr handling, close handler, and cleanup changes in your diff are already on main. This will have significant merge conflicts. Needs a rebase keeping only the `permissionMode` condition change.

Change 2: `claudeRemoteLauncher.ts` — improve error logging

Good fix. `JSON.stringify(new Error(...))` produces `'{}'` because Error properties are non-enumerable. Your explicit extraction of `e.name`, `e.message`, and `e.stack` is the correct approach. This change is clean and mergeable.

Change 3: `syncEngine.ts` — fallback to first online machine

This is risky. When `metadata.machineId` is set but doesn't match any online machine, falling back to `onlineMachines[0]` means the session resumes on a different machine. In multi-machine setups this can silently resume in the wrong context (wrong repo, wrong state).

The bot flagged the same issue and suggested a safer approach — only fall back when there's no machineId/host constraint:

```ts
if (metadata.machineId) {
const exact = onlineMachines.find((machine) => machine.id === metadata.machineId)
if (exact) return exact
return null // specific machine requested but not found
}
if (metadata.host) {
const hostMatch = onlineMachines.find((machine) => machine.metadata?.host === metadata.host)
if (hostMatch) return hostMatch
return null // specific host requested but not found
}
return onlineMachines[0] // no constraint, safe to fall back
```


Suggestions

  1. Split into separate PRs — the three changes are independent. A focused PR per fix would be easier to review and merge.
  2. Rebase Change 1 on current main — only the `permissionMode` condition is needed, the rest of the `query.ts` changes are already merged via #343.
  3. Rework Change 3 per the bot's suggestion to avoid the multi-machine safety issue.
  4. Change 2 can be submitted as-is — it's clean and self-contained.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants